Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Started the Convergence system #28848

Open
wants to merge 7 commits into
base: next
Choose a base branch
from

Conversation

joshuahansel
Copy link
Contributor

@joshuahansel joshuahansel commented Oct 14, 2024

This PR revives #26744.

This creates a new system: Convergence. See #24128.

I'm putting this up now so that I can document some TODOs. Because it's in a messy state, I plan to squash everything into one commit before merging.

Tasks that remain:

  • Consider any necessary changes for the checkConvergence() API. Currently I have iteration index as the one and only argument, since we may want to use the same convergence class for multiple types of convergence check, and different types of convergence check will need to obtain that index differently. The same argument can be applied to others. Also I think that some classes will only be compatible with certain types of convergence check. We would probably want to do some checking to make sure it's used in a compatible way. Perhaps we should have some kind of IterationType argument? I'm open to other ideas.
    UPDATE: Opened Make Convergence object checks aware of solve type #28927 for this task.
  • Understand the role of EquationSystems - does it do anything to set convergence parameters in there? Should Convergence being doing that? Some parameters in ResidualConvergence are unclear on if they're actually used, like the linear solve parameters. They get set in the EquationSystems object (which might be eventually set to PETSc), but at least some do not seem to come back into the class in the convergence check, maybe only getting used in libMesh's own convergence checks?
    UPDATE 1: The EquationSystems object is populated with the convergence-related parameters and then used in various convergence checks of different solvers, which do not all have the freedom to specify a generic convergence criteria. My plan is to not interact with the EquationSystems object from the Convergence classes if it is not necessary, and let the Executioner parameters populate it, as they do currently.
    UPDATE 2: The eigen executioners modify convergence-related parameters through EquationSystems, so it looks like we'll need to work with EquationSystems in the Convergence object after all.
  • Throw an error if the user supplies any shared convergence parameter to the executioner when they supply a ResidualConvergence? The current behavior is to allow the executioner parameters but override them. That's maybe the simplest implementation-wise, but seems very error-prone for users.
    UPDATE: I plan to give only a warning if the user specifies nonlinear convergence parameters in the executioner when they have a non-default convergence.
  • Improve PostprocessorConvergence to perform checks of execute_on. Here the Convergence would need to know what kind of iterations it will be used with, so we need to think about how to do this.
    Update: Opened Perform checks on execute_on for PostprocessorConvergence #28928 for this task.
  • Address any relevant comments from Convergence system #26744.

I opened another issue related to this: #28936.

@moosebuild
Copy link
Contributor

moosebuild commented Oct 15, 2024

Job Documentation, step Docs: sync website on 0920858 wanted to post the following:

View the site here

This comment will be updated on new commits.

@moosebuild
Copy link
Contributor

moosebuild commented Oct 16, 2024

Job Coverage, step Generate coverage on 0920858 wanted to post the following:

Framework coverage

af6bed #28848 092085
Total Total +/- New
Rate 85.09% 85.11% +0.01% 86.61%
Hits 106734 106924 +190 569
Misses 18696 18708 +12 88

Diff coverage report

Full coverage report

Modules coverage

Contact

af6bed #28848 092085
Total Total +/- New
Rate 90.45% 90.41% -0.05% 89.87%
Hits 4936 4957 +21 71
Misses 521 526 +5 8

Diff coverage report

Full coverage report

Full coverage reports

Reports

Warnings

  • framework new line coverage rate 86.61% is less than the suggested 90.0%
  • contact new line coverage rate 89.87% is less than the suggested 90.0%

This comment will be updated on new commits.

@joshuahansel
Copy link
Contributor Author

I'm considering renaming ResidualConvergence to FEProblemDefaultConvergence since it does more than check residual norms. It has tolerances for xnorm and snorm too, for example.

@joshuahansel
Copy link
Contributor Author

OpenMPI failure is a lie. Invalidate makes different tests fail.

@joshuahansel
Copy link
Contributor Author

@GiudGiud @lindsayad Ready for review

Copy link
Contributor

@GiudGiud GiudGiud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

quick part 1 review

you ll want to mention Convergence in NonlinearSystem.md at
## Convergence Criteria for the Nonlinear System

lots of functions longer than 6 lines 🤔

@joshuahansel
Copy link
Contributor Author

lots of functions longer than 6 lines

Most of those are inherited technical debt 😄 For example, there was a bunch of cut and paste from problems into convergences. My long-term plan here is to destroy these convergences in favor of smaller pieces. I see these convergences (e.g., FEProblemConvergence) as temporary objects for backwards-compatibility. Then, refactored, smaller convergences replace them in time.

@GiudGiud
Copy link
Contributor

Yes that makes sense. You already cut out the "min/max" iter based convergence from that object.

Copy link
Contributor

@GiudGiud GiudGiud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good.

@joshuahansel
Copy link
Contributor Author

@GiudGiud @lindsayad Ready for another round. I believe I've addressed your comments, so please check them off to confirm.

Copy link
Contributor

@GiudGiud GiudGiud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these and the equation system thing I asked on slack then good to go for me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants